Skip to content

Add option to canonicalise the version #18

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

fcollonval
Copy link
Member

@fcollonval fcollonval commented Jan 17, 2023

Context

Fixes #16

Code changes

For all cases, this adds a . between pre_l and pre_n for nodeJS version; this is recommended - see for example the behavior of .inc semver.

The user can also set a configuration flag canonical to True to convert Python package prerelease string to alpha, beta or rc in NodeJS (fallback to using the same string).

@fcollonval
Copy link
Member Author

Should make this option True by default?

@@ -9,18 +9,34 @@

GOOD_NODE_PYTHON_VERSIONS = [
("1.4.5", "1.4.5"),
("1.4.5-a0", "1.4.5a0"),
Copy link
Collaborator

@agoose77 agoose77 Jan 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right - a0 is parsed by semver as the pre-release named "a0", not "alpha" version "0". Good catch!

In fact, this shouldn't successfully parse. Can you change the NodeJS REGEX to make the . required if a pre-release numeral is given? i.e. (probably not this, but approximately)

(?P<pre_n>\.[0-9]+)?

This would mean that pre_n includes the delimiter in the match results.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current regex is

(?P<pre_l>(a|b|c|rc|alpha|beta|pre|preview))
        [-\.]?
        (?P<pre_n>[0-9]+)?

Is it ok to drop the dash? d2139b3 assumes so.

Copy link
Collaborator

@agoose77 agoose77 Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be explicit, these changes are to the general regex, rather than just the canonicalisation logic you're proposing.

  1. For Python's sake, I think we probably need to enforce a digit here, because semver treats 1.0.0-a < 1.0.0-a0. This means that these two versions are distinct for Node.js, but degenerate for Python.
  2. We need to drop the - because it would be considered part of the pre-release identifier, i.e. a|b|c|rc|alpha|beta|pre|preview, rather than an actual separator

I suppose the new pattern would be

(?P<pre_l>(a|b|c|rc|alpha|beta|pre|preview))
        \.
        (?P<pre_n>[0-9]+)

I'm using

for reference, but they're complex documents, so it's possible I will make mistakes. Let me know if you disagree!

@agoose77
Copy link
Collaborator

agoose77 commented Jan 17, 2023

Should make this option True by default?

I'd like to, yes. We already do not provide a guarantee of round-trip (unless you stick to a very specific scheme), and so it would be sensible to normalise in all cases. But, most of the users of this package probably don't have any bounds on it, so we would need to be careful not to break existing usage. @henryiii could use your opinion here, because this is a wider question about build backends.

We have two options for better compatibility of newer build backends with existing sdists in the wild:

  • Pin to e.g. minor version
  • Implement a version identifier that is declared in the configuration by the client package.

The former is easiest, and feels pretty natural. We'd encourage all users to set hatch_nodejs_version ~= 1.0 (after making such a release), and promise not to break in that window.

The latter means we can give newer code to more people, but I'm not sure if there's much merit.

In any case, right now we're fairly open.

@henryiii
Copy link

I'd generally avoid asking users to pin. If you do, you should commit to releasing v1 after you release v2 for as long as packages are still around pinning to v1. Build backends are fairly likely to need fixes as Python, OSs, etc. all evolve. Pinning it isn't something you can undo, so if you are stuck on a broken version, you are stuck. The Pythonic way to handle this is have a deprecation period & a warning. That doesn't mean users can't pin, but asking them to pin IMO is promising that pin will be supported in the future.

@agoose77
Copy link
Collaborator

@henryiii my worry is about whether we should make it feasible for old sdists to build on newer platforms, especially given how much activity there is in the build backend space (and plugin systems thereof) at the moment.

@fcollonval
Copy link
Member Author

Thanks for the quick review @agoose77

@agoose77
Copy link
Collaborator

agoose77 commented Jan 18, 2023

@fcollonval hmm, actually, my comments about our new REGEX may need revising Let me reset my thinking.

The challenge here is Node.js and Python handle version strings differently. Originally, I tried to be helpful and map reasonably between the two, but in practice is subjective, and perhaps not as predictable as I'd like.

Instead, I think we want to be able to guarantee that we have a one-to-one mapping for the limited scheme that we accept. This means that we could never accept all of

  • 1.0.0-a-0
  • 1.0.0-a.0
  • 1.0.0-a
  • 1.0.0-alpha

As they all map onto 1.0.0a0 in Python (canonical form). This is only because these are all different versions for Node.js. This makes me think that we should avoid all this flexibility, and enforce a strict scheme.

However, such a change would probably break existing users. I would want to make a 1.0.0 release to do this. Because we're already used by lots of packages, I'm not sure if first we would want to introduce a deprecation warning for this. Your thoughts are definitely welcome!

@fcollonval
Copy link
Member Author

I would take a pragmatic approach here. Although as you mentioned they are all different versions for Node.js. We can assume that a developer is consistent in its format. So there is a low chance it will change the version format in a way that does not change the Python version.

Moreover, if we advice in the documentation to prefer using the CLI hatch version <desired_version> to update the version rather than changing it by hand. There is even less possibilities of errors.

Note: We probably can document that edge case.

@agoose77
Copy link
Collaborator

agoose77 commented Feb 1, 2023

I just wanted to drop a line that things are crazy busy right now. I will try and look at this again soon. If this starts to block any Jupyter work, let me know, and I can either bump it in priority, or just bow out of the review.

@fcollonval
Copy link
Member Author

Friendly ping to agoose77

@agoose77
Copy link
Collaborator

agoose77 commented Sep 5, 2023

I remembered that this is still open today. I'll take another look at it soon, and figure out how best to proceed :)

@fleming79
Copy link

I'm experiment with source from this repo (ipylab). When I run

hatch version major,rc

It currently produces the following:

package.json

"version": "2.0.0-rc0"

_version.py

__version__ = VERSION = '2.0.0rc0'

Notice that the Python version is missing -.

Is the pull request likely to fix this round-trip problem?

@agoose77
Copy link
Collaborator

@fleming79 could you specify what you'd like to happen here? Python's pre-release segments should not be hyphenated (https://peps.python.org/pep-0440/#pre-releases).

@fleming79
Copy link

Is it possible to make them identical?

@agoose77
Copy link
Collaborator

@fleming79 I'm pretty sure the answer is no — the Python (PEP440) and SemVer specifications are incompatible. We support round-tripping by parsing PEP440 versions with a different pattern to SemVer.

@fleming79
Copy link

@agoose77 . Thank you for considering and the quick response. I guess in this case I could replace r with -r in the Python version to get the JavaScript version. But would it be possible to write the JavaScript version into the _version.py file (written by Hatch) so that it is easily accessible with Python?

@fcollonval
Copy link
Member Author

But would it be possible to write the JavaScript version into the _version.py file (written by Hatch) so that it is easily accessible with Python?

It is not needed as the package.json is copied within the Python package (the file is needed for lab extension by JupyterLab). In particular for your case, you can find it at ipylab/labextension/package.json (see the pyproject.toml serach for ensured-targets).

To answer more specifically your need, you can get the JS version in the ipylab/__init__.py with a snippet like:

import json
from pathlib import Path

js_version = None
try:
  HERE = Path(__file__).parent.resolve()
  with (HERE / "labextension" / "package.json").open() as fid:
      data = json.load(fid)
      js_version = data["version"]
except:
   ...

@fleming79
Copy link

I guess that works for dev, but the distribution puts that data somewhere totally different.

share\jupyter\labextensions\ipylab\package.json

See also a screenshot of the installed package (front right) and the unzipped wheel (back left).

image

It'd be more reliable if it was simply accessible by doing:

from ipylab._version import  __version__, __javascript_version__

Thank you anyway for the suggestions. In the interim I'll just use:

module_version = f"^{__version__}".replace("r", "-r")

@agoose77
Copy link
Collaborator

agoose77 commented Jan 19, 2024

@fleming79 thanks to you and @fcollonval, I now understand the potential use case for this. However, you should be able to use Hatch itself to do this.

By default, hatch-jupyter-builder (IIRC) bundles all JS assets under share/jupyter/labextensions/<PACKAGE-NAME>/. In order to make one of these files available to the Python package (under site-packages), you can instruct Hatch to include it using force-include: https://hatch.pypa.io/1.9/config/build/#forced-inclusion

Note that this works because we're asking Hatch to include a file twice, once as part of the Python "package", and once as part of the shared-data. Hatch doesn't let you copy files (at least to my knowledge), so we get two copies only because these are two separate inclusion mechanisms.

So, I'd suggest asking Hatch to include package.json under your Python package.

@fleming79
Copy link

Thank you both for your assistance.
Adding the following to the pyproject.toml includes the file in the distribution.

[tool.hatch.build.targets.wheel.force-include]
"ipylab/labextension/package.json" = "ipylab/labextension/package.json"

@agoose77
Copy link
Collaborator

@fleming79 fab! For the avoidance of doubt, I made a typo in my original reply ­— you both helped me to figure out what we should do here, it's appreciated.

@fcollonval
Copy link
Member Author

@agoose77 what should we do with this one? Maybe related to it, would you agree to transfer the owner ship of this to a oss community - like JupyterLab?

@agoose77
Copy link
Collaborator

@fcollonval I need to circle back on this and think about whether I still hold the same opinion.

Independently of that, I think we should move this to Jupyter stewardship because it's so widely used now; I really don't like that it's a single point of failure, even though I'm no longer the sole maintainer / owner! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add option to canonicalise the version
4 participants